Guard against pathologically connected dependency graphs #13424
Conversation
Since the DFS of the dependency graph is brute force it is exponential in the number of children. If a graph is pathological in nature (i.e. many cross-dependencies) then this will not complete in "reasonable" time. To address this we limit the number of visits to any node in the graph, thus capping the complexity to be linear in nature.
|
Again, thanks for contributing, as before, please bear in mind it might take some time for a maintainer to review this as we are all volunteers. But once I do get a chance I was going to test a few real world scenarios from simple to massive. If you need any assistance passing the pre-commit or CI let me know. |
Minor formatting fix. Added changelog entry.
|
Sure thing. I fixed the pre-merge checks, very simple. And in your own time. I am not blocked on this or anything so you can handle it as and when you deem fit. I have a day-job too, and also maintain some OSS, so I know the score. And, since I don't know if folks say this enough: Thanks for being a pip maintainer! |
There was a problem hiding this comment.
I'm pretty happy with this PR, in the sense that it doesn't add significant complexity and passes all the existing tests.
However, on testing real world examples I found very few that had any performance differences outside the margin of error. The one example I did find apache-airflow[all]<3 improved from ~0.009 seconds to ~0.002 seconds, which relatively is over a 75% improvement but absolutely, on my computer, is only 0.007 seconds.
Do you have a motivating example where this makes a big difference to you? You don't have to share if it's private, but it would be good to know what sort of numbers you are seeing. Or is this more of an academic exercise?
news/13424.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Handle pathologically connected dependency graphs in reasonable time. | |||
There was a problem hiding this comment.
Update this news item, it should be framed in the context of a pip user reading the changelog, e.g.
Minor performance improvement getting the order to install a very large number of interdependent packages.
Framed to be in the context of a pip user reading the changelog.
|
I updated the changelog item, mostly using your wording. |
Yeah, I would guess that this is likely to be expected; if this were a serious problem in the wild then no doubt it would have been seen and fixed by now. This issue only comes up for pathologically connected dependency graphs with cycles in them.
We're looking to move some internal, proprietary, code from one package management system to another. I was experimenting with using pip for some wheels which I had packaged up. The inter-package dependency tree is highly complex however, and has cycles in it. It turns out that the original visit mechanism's exponential-order graph walk wound up taking "forever"; per I could supply the dependency graph as a comment update to this PR if needed but, given its nature, it is pretty messy and would probably just be noise to the casual reader. FWIW, and with my tongue somewhat in my cheek, I will note that uv did not exhibit this issue. (Though it had other ones of its own.) So, this PR does a fair job in a "keeping up with the Joneses" sort of way. Finally, if the win is not deemed sufficient enough for the risk-cost of changing existing code, and the PR is bounced, then I would probably say it would fall into the class of me being "mildly sad" as opposed to "deeply miffed". It would not be the end of the world. |
|
Thanks for the info, it's not necessary to provide the graph, unless there's a good way to integrate it into the test suite. It's sufficient for me to know this does improve real world performance issues in a noticeable way. Pip is unlikely to ever catch up with the performance of uv, pip doesn't have any full time engineers, and is highly constrained by the user base from making large or risky changes. That said, it's always been my focus to fix performance bottlenecks in pip, and I'll be happy to see this merged. I'm going to leave this PR open for now to wait for any other maintainers to give feedback, and so I can take another pass over the algorithm. But unless any objections are raised I will aim to merge this ahead of 25.2. |
|
Sounds good and, again, no hurry from my perspective. As for catching up with uv, I would say that pip is probably fast enough for most people's needs. And totally agreed on the risk/benefit of making (any major) changes given the size of the user base, especially given that most of the wait-time is likely in IO at the end of the day anyhow. |
When determining the install order for dependencies the code does a DFS of the dependency graph, if it finds dependency cycle.
However, should the graph be pathologically connected, then this walk can take a log time, being exponential in nature. To mitigate this we limit the number of times any particular node may be visited during the walk. This should have little or no impact for well behaved graphs, while still yielding a fair approximation of a good installation order for densely connected graphs.
This is a less invasive version of #13416.